Skip to content

Update embedfs implementation #137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

this-kirke
Copy link

Fixes to embedfs implementation:

  • Fix path normalization to support billy's absolute path convention ("/")
  • Implement missing Lstat method (was returning ErrNotSupported)
  • Add proper embed.FS to billy path conversion throughout all methods

Additional test coverage:

  • Add dedicated test files for different functionality areas
  • Test File interface methods including ReadAt, Close, Lock/Unlock
  • Add path normalization and edge case testing
  • Test empty file handling and boundary conditions
  • Verify proper error handling for read-only operations

Test infrastructure:

  • Create embedfs_testdata package for reusable test data
  • Resolve import cycles with clean provider pattern
  • Add test data including nested directories and various file types
  • Organize tests by functionality (fs, dir, file operations)

The embedfs implementation now provides billy.Filesystem compliance for read-only embedded filesystems.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@this-kirke thanks for proposing these changes. Please take a look at the feedback below.

@this-kirke this-kirke force-pushed the feature/update-embedfs branch 4 times, most recently from 0f56f9c to 6ad6516 Compare July 27, 2025 03:51
@this-kirke this-kirke requested a review from pjbgf July 27, 2025 03:55
@this-kirke
Copy link
Author

Hi Paulo! Thank you for the review. I've updated with requested changes; all tests pass.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@this-kirke Thanks for the follow up. I just did a full review now, there are a few tests that are redundant which we can remove. But otherwise it LGTM.

Please squash your commits once you are done with the changes below.

@this-kirke
Copy link
Author

Thank you for the detailed review! I agree on all points. I think everything should be resolved now; please let me know if there's anything else you notice. We can squash on complete.

- Add chroot functionality to embedfs using billy's chroot helper, enabling compatibility with filesystem operations that require path isolation
- Fix path normalization to support billy's absolute path convention
- Implement Lstat method
- Additional tests to cover embedfs implementation
@this-kirke this-kirke force-pushed the feature/update-embedfs branch from 5cb6e4d to b89ce64 Compare August 4, 2025 17:42
@this-kirke this-kirke requested a review from pjbgf August 4, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants